azurerm_function_app - fix regressions in function app storage
#13580
Conversation
| @@ -356,7 +350,7 @@ func getBasicFunctionAppAppSettings(d *pluginsdk.ResourceData, appServiceTier, e | |||
| // On consumption and premium plans include WEBSITE_CONTENT components, unless it's a Linux consumption plan | |||
| // (see https://github.com/Azure/azure-functions-python-worker/issues/598) | |||
| if !(strings.EqualFold(appServiceTier, "dynamic") && strings.EqualFold(d.Get("os_type").(string), "linux")) && | |||
| (strings.EqualFold(appServiceTier, "dynamic") || strings.HasPrefix(strings.ToLower(appServiceTier), "elastic")) { | |||
There was a problem hiding this comment.
Just looking to confirm that we don't need to check for elastic anymore? Is there any chance someone has that still?
There was a problem hiding this comment.
As far as I know, Azure Files is only required for Consumption (Windows) and Elastic Premium (Windows / Linux). I think it is correct to leave this conditional expression as elastic. See also #13218 (comment)
There was a problem hiding this comment.
There was a problem hiding this comment.
This is very confusing, but the Premium V2 / V3 of the App Service Plan and the Azure Functions Premium Plan are completely different services. The Premium Plan in this document refers to the latter.
If the File Share setting is added to the App Service Plan (Premium V2 / V3), the current working application may be lost. This is a very dangerous change and the application I manage is also affected.
There was a problem hiding this comment.
This is very confusing,
You are not wrong there. 🙃
The resource has always previously sent the content details for premium SKU's, and elasticpremium plans see here
The change in 2.77 was intended to align the resource behaviour with the docs, but appears to have created the issue in 13218 where these settings were being sent where they had previously not been. Are you reporting that the Premium plans did not get these settings as intended?
The other change in this PR addresses the change of setting for the share directory, which is a bug in the 2.77 implementation which attempted to maintain any existing value configured. (see 13536) If the property already exists in the Function App AppSettings, then it will be used and not added/created. Additionally this is intended to address a related bug whereby the provider created the share with a -content suffix that prevented proper operation of slots. This will also mean that if the share property is set for a Premium Plan, it will be honoured/maintained and not overwritten.
There was a problem hiding this comment.
I think your change in #13349 is correct. I actually created and tested the resource, and it behaved as intended. Isn't the only thing this PR needs to do is fix the problem of WEBSITE_CONTENTSHARE not being reused?
The only thing I would strongly argue is that the App Service Plan (Premium V2 / Premium V3) does not require Azure Files configuration. At least, when I tested using this branch's code against Azure Functions running on Premium V2, I saw that unnecessary Azure Files settings (WEBSITE_CONTENTSHARE and WEBSITE_CONTENTAZUREFILECONNECTIONSTRING) were added and the application was lost. I can share the definition if you want to reproduce it on your device.
In my opinion, for each tier (Consumption / App Service Plan / Premium Plan), the test cases need to be checked for the existence of WEBSITE_CONTENTSHARE and WEBSITE_CONTENTAZUREFILECONNECTIONSTRING.
The resource has always previously sent the content details for premium SKU's, and elasticpremium plans see here
Yes, that change will break your application if you are already using Premium V2 / V3. I have confirmed that it breaks with this comment. This buggy code has been fixed in v2.77.
There was a problem hiding this comment.
As for #13566, it is different from this issue.
Azure Functions has an option to operate without File Share, so I think it's more appropriate to add a property (e.g. disabled_builtin_file_share or etc) to not auto-set WEBSITE_CONTENTSHARE and WEBSITE_CONTENTAZUREFILECONNECTIONSTRING.
There was a problem hiding this comment.
Thanks for your insight @shibayan - I'm taking this back to the drawing board to work it back through...
There was a problem hiding this comment.
I've updated to correct the behaviour for PremiumV2/V3 (i.e. not send) and added the reuse of an existing value if defined (in case users do have it defined). I believe this is going to be sufficient for this stage, as presently this resource is not usable since 2.77 because of the share recreation bug. I'm reticent to introduce another property at this stage that will further affect behaviour of the resource. Upon re-reading, I believe 13566 to be a symptom of the share recreation bug, so should be resolved by this PR in any case.
fwiw - I'm in the process of completely rewriting this resource for the beta / v3.0, so I've made notes for addressing this properly there. I'll also be speaking to the Service Team on this as soon as possible.
There was a problem hiding this comment.
Thank you for the quick response. I've checked the changed code and I'm sure it works ideally with Provider v2.x.
|
Tests looking OK, (2 transient / unrelated failures) |
|
This functionality has been released in v2.80.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
fixes #13536
fixes #13566